Skip to content

jsonrpc: Add async support#558

Open
jamillambert wants to merge 5 commits intorust-bitcoin:masterfrom
jamillambert:0417-jsonrpc-async
Open

jsonrpc: Add async support#558
jamillambert wants to merge 5 commits intorust-bitcoin:masterfrom
jamillambert:0417-jsonrpc-async

Conversation

@jamillambert
Copy link
Copy Markdown
Collaborator

Pulled out of #505 and polished.

The aim is to add async support without breaking existing sync downstream. This includes keeping the reexports of the current sync Client at the crate root.

Copy link
Copy Markdown
Contributor

@satsfy satsfy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 5906ce1 - Ran some async tests locally with the new client.


impl BitreqHttpTransport {
/// Constructs a new [`BitreqHttpTransport`] with default parameters.
pub fn new() -> Self { BitreqHttpTransport::default() }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment. Should we consider some validation here? So that the user cannot shoot themselves on the foot like:

use jsonrpc::bitreq_http_async::Builder;
Builder::new().url("not_a_url").is_ok();

I suppose a validation could happen at new or at fn build(self). No change required, because sync client also does it too. But it's a thought for production.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raise an issue man, your thoughts are valuable and will be lost if left here!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth raising an issue. But not_a_url is a valid URL. I'm not sure what validation would be reasonable.

Copy link
Copy Markdown
Contributor

@satsfy satsfy Apr 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks I'll put it on the production client's existing issue as an additional consideration.

@satsfy have you spent much time with async code?

@tcharding I don't have enough async experience for a decisive review.

Copy link
Copy Markdown
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 5906ce1 - but I know nothing about async code.

@tcharding
Copy link
Copy Markdown
Member

Since we are going to cut a release that includes code I know nothing about (async) I'd love to see PRs up that test everything before we release. Would you consider doing:

  • A release tracking PR for jsonrpc (bumps version and updates local crates that depend on it).
  • Rebase the prod RPC client and use the branch/PR created above

Then with green CI on those two PRs we'd have confidence we got things right and we can go ahead and cut the releases.

And if someone with async experience has ten minutes to cast an eye over 5906ce1 please that would be very much appreciated.

@tcharding
Copy link
Copy Markdown
Member

@satsfy have you spent much time with async code?

@apoelstra
Copy link
Copy Markdown
Member

I have a reasonable amount of experience with async. I'll take a look.

pub fn builder() -> Builder { Builder::new() }

fn request<R>(&self, req: impl serde::Serialize) -> Result<R, Error>
/// Returns the timeout in whole seconds, rounding positive sub-second values up to one.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 5906ce1:

This is fine, but shouldn't it be done in a separate commit and be done for the sync version too?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a commit that changes it in the sync client, this is before the copy of the code to async so it then carries across.

Comment thread jsonrpc/src/http/bitreq_http_async.rs Outdated
&'a self,
req: Request<'a>,
) -> BoxFuture<'a, Result<Response, crate::Error>> {
Box::pin(async move { Ok(self.request(req).await?) })
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 5906ce1:

This mess can be replaced with Box::pin(self.request(req))

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better, thanks.

Comment thread jsonrpc/src/http/bitreq_http_async.rs Outdated
&'a self,
reqs: &'a [Request<'a>],
) -> BoxFuture<'a, Result<Vec<Response>, crate::Error>> {
Box::pin(async move { Ok(self.request(reqs).await?) })
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 5906ce1:

...and this one with Box::pin(self.request(reqs))

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread jsonrpc/src/client_async.rs Outdated
fn send_request(&self, _: Request) -> Result<Response, Error> { Err(Error::NonceMismatch) }
fn send_batch(&self, _: &[Request]) -> Result<Vec<Response>, Error> { Ok(vec![]) }
fn send_request<'a>(&'a self, _: Request<'a>) -> BoxFuture<'a, Result<Response, Error>> {
Box::pin(async { Err(Error::NonceMismatch) })
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 5906ce1:

This is just a style thing but you can write Box::pin(futures::err(Error::NonceMismatch)) here. It might even compile faster or more efficiently.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread jsonrpc/src/client_async.rs Outdated
&'a self,
_: &'a [Request<'a>],
) -> BoxFuture<'a, Result<Vec<Response>, Error>> {
Box::pin(async { Ok(vec![]) })
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 5906ce1:

Similarly, futures::ok(vec![])

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread jsonrpc/src/client_async.rs Outdated
pub fn send_request(&self, request: Request) -> Result<Response, Error> {
self.transport.send_request(request)
pub async fn send_request(&self, request: Request<'_>) -> Result<Response, Error> {
self.transport.send_request(request).await
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 5906ce1:

It might not be worth the abstraction leak, but you can change this method to return a BoxFuture and then you don't need async or await (and the resulting code will be smaller and faster to compile since it saves you a dummy state machine).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to return a BoxFuture.

Thanks for the review it was very helpful.

Timeouts must be whole seconds for some HTTP clients like bitreq.

Round timeouts to whole seconds to avoid issues with these clients.
Code copy only to make it easier in the next patch to see what the
changes are for async.
Remove code related to fuzzing from the new async files. Fuzzing of the
async client can be added back later if needed.
In preparation for adding another async client change the Debug impl to
refer to the local sync Client so that there is no conflict when adding
the second async Client struct.
@jamillambert jamillambert force-pushed the 0417-jsonrpc-async branch 2 times, most recently from 3d47015 to 559e8e3 Compare April 20, 2026 19:51
Update the lockfiles because of the new dev dependency on futures.
@jamillambert
Copy link
Copy Markdown
Collaborator Author

Made all of the suggested changes.

@apoelstra
Copy link
Copy Markdown
Member

utACK 0aa7803

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants